Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase PHPStan level to 5 #244

Merged
merged 3 commits into from
Sep 10, 2024
Merged

Increase PHPStan level to 5 #244

merged 3 commits into from
Sep 10, 2024

Conversation

staudenmeir
Copy link
Owner

@staudenmeir staudenmeir commented Sep 9, 2024

There is only one type of PHPStan error:

Parameter #1 $relations of method Illuminate\Database\Eloquent\Collection<int,Tests\Concatenation\LaravelAdjacencyList\Models\User>::load() expects array<(callable(Illuminate\Database\Eloquent\Builder<Tests\Concatenation\LaravelAdjacencyList\Models\User>): mixed)|string>|string, array{ancestorPosts: Closure(Staudenmeir\EloquentHasManyDeep\HasManyDeep): Staudenmeir\EloquentHasManyDeep\HasManyDeep<Illuminate\Database\Eloquent\Model>} given.

The affected code looks like this:

$users = User::all()->load([
    'ancestorPosts' => fn (HasManyDeep $query) => $query->orderBy('id'),
]);

The first issue is that the parameter type of Collection::load() is incorrect. The callable function actually gets passed a relationship, not an Eloquent builder:

* @param  array<array-key, (callable(\Illuminate\Database\Eloquent\Relations\Relation): mixed)|string>|string  $relations

However, this fix doesn't resolve the error:

Parameter #1 $relations of method Illuminate\Database\Eloquent\Collection<int,Tests\Concatenation\LaravelAdjacencyList\Models\User>::load() expects array<(callable(Illuminate\Database\Eloquent\Relations\Relation):
mixed)|string>|string, array{ancestorPosts: Closure(Staudenmeir\EloquentHasManyDeep\HasManyDeep): Staudenmeir\EloquentHasManyDeep\HasManyDeep<Illuminate\Database\Eloquent\Model>} given.

The error only goes away when you change the parameter's type to Relation (despite HasManyDeep being a subclass of Relation):

$users = User::all()->load([
    'ancestorPosts' => fn (Relation $query) => $query->orderBy('id'),
]);

With this change, however, PHPStan can't handle methods calls anymore despite the Relation class's Eloquent builder mixin:

Call to an undefined method Illuminate\Database\Eloquent\Relations\Relation::orderBy().

@calebdw As discussed.

@SanderMuller Maybe you have an idea?

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@staudenmeir
Copy link
Owner Author

staudenmeir commented Sep 9, 2024

@calebdw Regarding the PHPDoc fix for Collection::load(): I used ...Relations\Relation): mixed... instead of ...Relations\Relation<*, *, *>): mixed... because PhpStorm can't parse the asterisks. Not sure if they're invalid here or it's just a PhpStorm bug.

@calebdw
Copy link

calebdw commented Sep 9, 2024

@staudenmeir, the asterisks are valid, higher levels of PHPStan are going to require that the inner types are defined.
* in this case is a wildcard that just means a relation with any inner type is accepted

@calebdw
Copy link

calebdw commented Sep 9, 2024

The problem is that the array is being parsed incorrectly and it thinks an array<callable> is expected instead of an array<array-key, callable>.

Try to update to:

// remove the space
/** @param  array<array-key,(callable(\Illuminate\Database\Eloquent\Relations\Relation): mixed)|string>|string  $relations */

// or try wrapping the array in parens
/** @param  (array<array-key, (callable(\Illuminate\Database\Eloquent\Relations\Relation): mixed)|string>)|string  $relations */

If this doesn't work then it's likely a phpstan parse error, try to replicate on https://phpstan.org/try and open an issue with phpstan

@staudenmeir
Copy link
Owner Author

Thanks @calebdw. These updates don't fix the error, unfortunately.

@calebdw
Copy link

calebdw commented Sep 9, 2024

Actually, this looks like it's because parameter types are not covariant which means you can't define a narrower type:

https://phpstan.org/r/0e4afb98-6c78-4076-9b50-5035cb0d8faf

The solution is to either use Relation or just don't type the closure (I tend not to add types to framework closures):

$users = User::all()->load([
    'ancestorPosts' => fn (Relation $query) => $query->orderBy('id'),
]);
// or
$users = User::all()->load([
    'ancestorPosts' => fn ($query) => $query->orderBy('id'),
]);

@staudenmeir
Copy link
Owner Author

Thanks @calebdw. I typed the closures to get autocompletion from PhpStorm.

Do you know why the Eloquent builder mixin doesn't work here with fn (Relation $query)?

Call to an undefined method Illuminate\Database\Eloquent\Relations\Relation::orderBy().

@calebdw
Copy link

calebdw commented Sep 9, 2024

@staudenmeir, the mixin is working fine when I add it to the framework tests. The issue may be that we're still waiting on larastan/larastan#1990 to be merged so that Larastan supports Laravel >= 11.15

@SanderMuller
Copy link
Contributor

@staudenmeir I also had to ignore this error in my other projects where I went to level 5/6

I've created a PR to fix this: #245
It also adds the phpstan/phpstan-phpunit extension

@staudenmeir staudenmeir marked this pull request as ready for review September 10, 2024 14:52
@staudenmeir
Copy link
Owner Author

When laravel/framework#52724 gets released, we can remove the ignored error.

@staudenmeir
Copy link
Owner Author

Thanks @calebdw and @SanderMuller.

@staudenmeir staudenmeir merged commit 39891e6 into main Sep 10, 2024
26 checks passed
@staudenmeir staudenmeir deleted the phpstan-l5 branch September 10, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants